refactor: optimize cache key type for resolve_to_key#6961
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
WalkthroughMessage-pool address-resolution now uses finality-stable deterministic resolution and an ID-keyed cache: cache keys changed from Changes
Sequence Diagram(s)sequenceDiagram
participant MP as MessagePool
participant Cache as KeyCache (AddressId -> Address)
participant Prov as Provider
participant SM as StateManager / ChainStore
MP->>Cache: lookup(addr.id() -> AddressId)
alt cache hit
Cache-->>MP: cached Address
else cache miss
MP->>Prov: resolve_to_deterministic_address_at_finality(addr, tipset)
Prov->>SM: compute lookback state & resolve deterministically
SM-->>Prov: deterministic Address
Prov-->>MP: deterministic Address
MP->>Cache: insert(AddressId, Address)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/message_pool/msgpool/provider.rs (1)
122-132:⚠️ Potential issue | 🟠 MajorReturn an error until a finality lookback tipset exists.
This branch currently resolves against
tswhen the chain is younger thanchain_finality, which contradicts the contract on Lines 51-52 and lets the mpool cache mappings that are not finality-stable yet. Please fail fast here instead of treating the current head as final.Suggested fix
- let lookback_ts = if ts.epoch() > self.chain_config().policy.chain_finality { + let lookback_ts = if ts.epoch() > self.chain_config().policy.chain_finality { self.chain_index() .tipset_by_height( ts.epoch() - self.chain_config().policy.chain_finality, ts.clone(), ResolveNullTipset::TakeOlder, ) .map_err(|e| Error::Other(e.to_string()))? } else { - ts.clone() + return Err(Error::Other( + "cannot resolve ID address to a finality-stable deterministic address yet" + .into(), + )); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message_pool/msgpool/provider.rs` around lines 122 - 132, The branch that sets lookback_ts currently uses ts.clone() when ts.epoch() <= self.chain_config().policy.chain_finality; instead, fail fast by returning an error so we don't cache non-finality-stable mappings: in the function that computes lookback_ts (referencing lookback_ts, self.chain_config().policy.chain_finality, and self.chain_index().tipset_by_height with ResolveNullTipset), replace the else arm with an early Err(...) return (e.g., using Error::Other with a clear message) indicating that a finality lookback tipset does not yet exist rather than using ts.clone().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/message_pool/msgpool/provider.rs`:
- Around line 122-132: The branch that sets lookback_ts currently uses
ts.clone() when ts.epoch() <= self.chain_config().policy.chain_finality;
instead, fail fast by returning an error so we don't cache non-finality-stable
mappings: in the function that computes lookback_ts (referencing lookback_ts,
self.chain_config().policy.chain_finality, and
self.chain_index().tipset_by_height with ResolveNullTipset), replace the else
arm with an early Err(...) return (e.g., using Error::Other with a clear
message) indicating that a finality lookback tipset does not yet exist rather
than using ts.clone().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d390790-a546-431e-a2da-70b4fa4adcfa
📒 Files selected for processing (6)
src/message_pool/msgpool/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/provider.rssrc/message_pool/msgpool/selection.rssrc/message_pool/msgpool/test_provider.rssrc/state_manager/mod.rs
c498ce7 to
0660f70
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/message_pool/msgpool/provider.rs (1)
122-140: Add operation-specific context to error mapping.At Line 129, Line 137, and Line 140, converting errors with
e.to_string()drops step-level context, which makes runtime diagnosis harder.Suggested patch
- let lookback_ts = if ts.epoch() > self.chain_config().policy.chain_finality { + let finality = self.chain_config().policy.chain_finality; + let lookback_ts = if ts.epoch() > finality { self.chain_index() .tipset_by_height( - ts.epoch() - self.chain_config().policy.chain_finality, + ts.epoch() - finality, ts.clone(), ResolveNullTipset::TakeOlder, ) - .map_err(|e| Error::Other(e.to_string()))? + .map_err(|e| { + Error::Other(format!( + "failed loading lookback tipset at epoch {}: {e}", + ts.epoch() - finality + )) + })? } else { // Matches the logic at <https://github.com/filecoin-project/lotus/blob/v1.35.1/chain/stmgr/stmgr.go#L361> ts.clone() }; let state = StateTree::new_from_root(self.blockstore().clone(), lookback_ts.parent_state()) - .map_err(|e| Error::Other(e.to_string()))?; + .map_err(|e| Error::Other(format!("failed building state tree: {e}")))?; state .resolve_to_deterministic_addr(self.blockstore(), *addr) - .map_err(|e| Error::Other(e.to_string())) + .map_err(|e| { + Error::Other(format!( + "failed resolving deterministic address at finality for {addr}: {e}" + )) + })As per coding guidelines, “Use
anyhow::Result<T>for most operations and add context with.context()when errors occur”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/message_pool/msgpool/provider.rs` around lines 122 - 140, The error mappings around the lookback/tipset and state resolution drop useful context; update the calls that currently use map_err(|e| Error::Other(e.to_string())) for tipset_by_height, StateTree::new_from_root, and state.resolve_to_deterministic_addr to add operation-specific context (e.g., using anyhow::Context and .context("...") or otherwise wrapping the error with a message) so failures include which step failed (computing lookback_ts via tipset_by_height, constructing StateTree from parent_state, or resolving deterministic address) and relevant values (epoch/chain_finality, parent_state, addr); adjust the function signature to return anyhow::Result if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/message_pool/msgpool/provider.rs`:
- Around line 122-140: The error mappings around the lookback/tipset and state
resolution drop useful context; update the calls that currently use map_err(|e|
Error::Other(e.to_string())) for tipset_by_height, StateTree::new_from_root, and
state.resolve_to_deterministic_addr to add operation-specific context (e.g.,
using anyhow::Context and .context("...") or otherwise wrapping the error with a
message) so failures include which step failed (computing lookback_ts via
tipset_by_height, constructing StateTree from parent_state, or resolving
deterministic address) and relevant values (epoch/chain_finality, parent_state,
addr); adjust the function signature to return anyhow::Result if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bfa04351-eada-43f5-82a3-cbbe759a7519
📒 Files selected for processing (6)
src/message_pool/msgpool/mod.rssrc/message_pool/msgpool/msg_pool.rssrc/message_pool/msgpool/provider.rssrc/message_pool/msgpool/selection.rssrc/message_pool/msgpool/test_provider.rssrc/state_manager/mod.rs
✅ Files skipped from review due to trivial changes (1)
- src/state_manager/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/message_pool/msgpool/test_provider.rs
- src/message_pool/msgpool/mod.rs
- src/message_pool/msgpool/msg_pool.rs
- src/message_pool/msgpool/selection.rs
Co-authored-by: Copilot <copilot@github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary of changes
Follow up on https://github.com/ChainSafe/forest/pull/6910/changes#r3109534232
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Refactor
Documentation